413 improve private key management in bls provider and signer #548

JohnGuilding · 2023-03-08T16:52:57Z

What is this PR doing?

This PR removes all references to the signer in the BlsProvider. This reduces the tight coupling between the two classes, better represents a JsonRpcProvider, and we can remove code like:

if (!this.signer) {
      throw new Error("Call provider.getSigner first");
}

Other changes:

Moved the constructTransactionResponse and constructTransactionBatchResponse methods to the provider. This consolidates the logic in one place.
Added _validateTransaction and _validateTransactionBatch methods in the signer to split validation logic. This reduces the complexity of the send transaction methods and makes testing easier.
Moved some tests from integration tests to ./clients as they don't require a running network to pass.
Removed some regular JsonRpcProvider and JsonRpcSigner tests at the bottom of the BlsProvider.test.ts and BlsSigner.test.ts files.
Cleans up provider and signer code and test code

How can these changes be manually tested?

run integration tests and clients tests

Does this PR resolve or contribute to any issues?

#413
Also fixes #427. Turns out I wasn't funding the new signer in the test.

Checklist

I have manually tested these changes
Post a link to the PR in the group chat

Guidelines

If your PR is not ready, mark it as a draft
The resolve conversation button is for reviewers, not authors
- (But add a 'done' comment or similar)

JohnGuilding · 2023-03-16T17:27:32Z

contracts/clients/src/BlsProvider.ts

    );

    const actionWithFeePaymentAction =
      this._addFeePaymentActionForFeeEstimation([action]);

+    const throwawayPrivateKey = await BlsWalletWrapper.getRandomBlsPrivateKey();


Open to suggestions on being able to estimate fees in the provider without having reference to a signer. This was my hacky solution. It does result in slightly different gas estimates each time, but still close enough to send the transaction successfully.

See this comment for examples of differences in tests without mocking a consistent private key value.

Not a fan of creating a throwaway BlsWalletWrapper, but feels slightly better than having a signer tightly coupled to a provider.

I think for now this is fine. I would document why your are doing this in code.

We should have an estimation endpoint/method for v2 that does not require a signed bundle.

Added a comment and tracked via a follow up issue for V2 #560

JohnGuilding · 2023-03-16T17:34:01Z

contracts/test-integration/BlsSigner.test.ts

@@ -567,6 +510,14 @@ describe("BlsSigner", () => {
      blsSigner,
    );

+    // BlsWalletWrapper.getRandomBlsPrivateKey from "estimateGas" method results in slightly different
+    // fee estimates. Which leads to a different signature. This fake avoids signature mismatch by stubbing a constant value.
+    sinon.replace(


Example of differences observed without sinon.fake:

Test Expected Fee Estimate Fee Estimate

1 0.000079145258744232 0.000077749364772554

2 0.00006806648707833 0.000069299213638008

3 0.000060669559559918 0.000060666067046826

JohnGuilding · 2023-03-16T17:42:12Z

contracts/clients/src/BlsProvider.ts

+
+    const transactions: Array<ethers.providers.TransactionResponse> = [];
+
+    for (const publicKeyLinkedToActions of publicKeysLinkedToActions) {


It is possible in the provider to get a bundle with multiple public keys and operations, this logic ensures all of the transactions in transactions: Array<ethers.providers.TransactionResponse> = [] are linked to the correct nonce and from address.

e.g. if I just mapped across all of the actions like I was before, then there would be no way of telling which public key was linked to which group of actions.

JohnGuilding · 2023-03-16T17:50:28Z

contracts/clients/src/BlsSigner.ts

+    }
+
+    if (!resolvedTransaction.from) {
+      resolvedTransaction.from = await this.getAddress();


This is a subtle, but important change, it ensures we don't require the following logic in many places and have to enforce adding from to transaction requests:

if (!resolvedTransaction.from) { throw new TypeError("Transaction.from should be defined"); }

This is because the provider estimateGas method now requires from to calculate the nonce, and the transaction that this method is validating will get pass to that method later in the send transaction methods. See changes to estimateGas and this comment for more context

JohnGuilding · 2023-03-16T17:53:05Z

contracts/test-integration/BlsProvider.test.ts

-      value: transactionAmount,
+      to: ethers.Wallet.createRandom().address,
+      value: parseEther("1"),
+      from: getAddressPromise,


from needed to be added manually here as calling estimateGas directly. The _validateTransaction method on the signer does this work for us in cases where the transaction comes from the signer.

github-actions bot added aggregator Aggregator backend related aggregator-proxy Aggregator proxy related clients contracts Smart contract related documentation Improvements or additions to documentation extension Browser extension related labels Mar 8, 2023

JohnGuilding force-pushed the 413-improve-private-key-management-in-bls-provider-and-signer branch from cdafd92 to e73949a Compare March 14, 2023 17:54

github-actions bot removed aggregator Aggregator backend related aggregator-proxy Aggregator proxy related documentation Improvements or additions to documentation extension Browser extension related labels Mar 14, 2023

JohnGuilding added 2 commits March 16, 2023 16:12

Remove reference to signer from provider

e3bbd39

Cleanup test code & add contract interaction gas estimate tests

32c6b13

JohnGuilding force-pushed the 413-improve-private-key-management-in-bls-provider-and-signer branch from e73949a to 32c6b13 Compare March 16, 2023 17:01

re-add accidentally deleted tests

3c0f36f

JohnGuilding force-pushed the 413-improve-private-key-management-in-bls-provider-and-signer branch from 33ab665 to 3c0f36f Compare March 16, 2023 17:20

JohnGuilding commented Mar 16, 2023

View reviewed changes

JohnGuilding marked this pull request as ready for review March 16, 2023 18:08

jacque006 approved these changes Mar 21, 2023

View reviewed changes

JohnGuilding mentioned this pull request Mar 22, 2023

Estimate fee without requiring a signed bundle #560

Closed

Add comment explaining throwaway BlsWalletWrapper

5423f65

JohnGuilding merged commit ac7cd95 into main Mar 22, 2023

JohnGuilding deleted the 413-improve-private-key-management-in-bls-provider-and-signer branch March 22, 2023 19:20

jacque006 mentioned this pull request Mar 28, 2023

Move references to private key outside of BlsSigner #413

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

413 improve private key management in bls provider and signer #548

413 improve private key management in bls provider and signer #548

JohnGuilding commented Mar 8, 2023 •

edited

Loading

JohnGuilding Mar 16, 2023 •

edited

Loading

jacque006 Mar 21, 2023

JohnGuilding Mar 22, 2023

JohnGuilding Mar 16, 2023

JohnGuilding Mar 16, 2023

JohnGuilding Mar 16, 2023 •

edited

Loading

JohnGuilding Mar 16, 2023 •

edited

Loading

Test	Expected Fee Estimate	Fee Estimate
1	0.000079145258744232	0.000077749364772554
2	0.00006806648707833	0.000069299213638008
3	0.000060669559559918	0.000060666067046826


		const transactions: Array<ethers.providers.TransactionResponse> = [];

		for (const publicKeyLinkedToActions of publicKeysLinkedToActions) {

413 improve private key management in bls provider and signer #548

413 improve private key management in bls provider and signer #548

Conversation

JohnGuilding commented Mar 8, 2023 • edited Loading

What is this PR doing?

How can these changes be manually tested?

Does this PR resolve or contribute to any issues?

Checklist

Guidelines

JohnGuilding Mar 16, 2023 • edited Loading

Choose a reason for hiding this comment

jacque006 Mar 21, 2023

Choose a reason for hiding this comment

JohnGuilding Mar 22, 2023

Choose a reason for hiding this comment

JohnGuilding Mar 16, 2023

Choose a reason for hiding this comment

JohnGuilding Mar 16, 2023

Choose a reason for hiding this comment

JohnGuilding Mar 16, 2023 • edited Loading

Choose a reason for hiding this comment

JohnGuilding Mar 16, 2023 • edited Loading

Choose a reason for hiding this comment

JohnGuilding commented Mar 8, 2023 •

edited

Loading

JohnGuilding Mar 16, 2023 •

edited

Loading

JohnGuilding Mar 16, 2023 •

edited

Loading

JohnGuilding Mar 16, 2023 •

edited

Loading